test(ack-id): add comprehensive A2A module test suite#69
test(ack-id): add comprehensive A2A module test suite#69ak68a wants to merge 3 commits intoagentcommercekit:mainfrom
Conversation
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add test-fixtures.ts with shared identity constants, message builders, and mock factories - Replace inline message construction with typed builder functions - Use it.each for schema rejection cases - Derive expected signed payload from signedMessage() to prevent fixture drift - Add unsignedMessage() builder for explicit unsigned test cases - Add comments explaining why (contextId stripping, schema-first validation, nonce correlation) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds five new Vitest test files and a shared test-fixtures module covering random ID generation, service endpoint construction, A2A handshake/message signing, and verification behavior for handshake and signed messages. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
packages/ack-id/src/a2a/verify.test.ts (1)
84-112: Enforce schema-first behavior with an explicitverifyJwtnon-call assertion.The table test currently checks rejection only; it should also prove malformed input is rejected before JWT verification.
Suggested update
])("rejects $name", async ({ message }) => { + vi.mocked(verifyJwt).mockClear() await expect( verifyA2AHandshakeMessage(message as never, { did: agentDid }), ).rejects.toThrow() + expect(verifyJwt).not.toHaveBeenCalled() })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ack-id/src/a2a/verify.test.ts` around lines 84 - 112, The tests in verify.test.ts currently only assert that malformed messages cause verifyA2AHandshakeMessage to reject, but they don’t assert that JWT verification (verifyJwt) is not invoked; update the table-driven test to mock or spy on the verifyJwt function used by verifyA2AHandshakeMessage and add a non-call assertion (e.g., expect(verifyJwt).not.toHaveBeenCalled()) after the await expect(...).rejects.toThrow() so the test proves schema validation occurs before any JWT verification. Make sure the mock/spy targets the same symbol imported/used by verifyA2AHandshakeMessage (verifyJwt) so the assertion reliably detects no calls.packages/ack-id/src/a2a/random.test.ts (1)
5-6: Consider asserting UUIDv4-specific bits in the regex.Current pattern validates generic UUID shape only. If
generateRandomJti/generateRandomNonceare expected to be UUIDv4, tightening this improves mutation resistance.Suggested tweak
-const uuidPattern = - /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/ +const uuidPattern = + /^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ack-id/src/a2a/random.test.ts` around lines 5 - 6, The current uuidPattern only checks generic UUID shape; tighten it to assert UUIDv4 version and RFC-variant bits so tests for generateRandomJti and generateRandomNonce validate v4 UUIDs. Replace the pattern used in uuidPattern with one that enforces a '4' in the version position and one of [89ab] in the variant position (e.g. pattern matching ...-4...-[89ab]...-...), keeping case handling consistent with the rest of the tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/ack-id/src/a2a/service-endpoints.test.ts`:
- Around line 19-26: Rename the test title to use the approved assertive
pattern: change the it(...) description for the test that calls
createAgentCardServiceEndpoint with DID "did:web:example.com:agents:my-agent" to
an assertive form such as "returns an id with `#agent-card` for colon-separated
DID paths" (or another "creates/returns/throws/requires" phrasing); keep the
test body and expectations
(expect(endpoint.id).toBe("did:web:example.com:agents:my-agent#agent-card"))
unchanged so only the test name is updated.
In `@packages/ack-id/src/a2a/sign-message.test.ts`:
- Around line 45-151: Rename the test titles to use assertive verbs
(creates/returns/throws/requires) so they follow the project's naming
convention: update the it(...) strings in tests referencing
createA2AHandshakePayload ("echoes the request nonce..." → "returns the request
nonce and creates a new reply nonce"), createA2AHandshakeMessageFromJwt ("wraps
a JWT..." → "returns an A2A data-part message containing a JWT" and "respects
the role parameter" → "returns the provided role"), createSignedA2AMessage
("produces a JWT signature..." → "creates a JWT signature and attaches it to
message metadata", "preserves original message content..." → "returns original
message content alongside the signature", "merges the signature..." → "creates
merged metadata without clobbering existing fields"), and
createA2AHandshakeMessage ("signs a credential handshake and returns the
nonce..." → "creates a signed credential handshake and returns the nonce",
"embeds the signed JWT..." → "returns a message embedding the signed JWT"); keep
the test logic and assertions unchanged, only change the human-readable it(...)
description strings.
In `@packages/ack-id/src/a2a/verify.test.ts`:
- Around line 40-250: The tests use non-assertive "it(...)" titles; rename each
test title in this suite to use the mandated assertive prefixes (creates...,
throws..., requires..., or returns...) so they conform to the guideline.
Specifically update the it(...) descriptions in the verifyA2AHandshakeMessage
and verifyA2ASignedMessage blocks (tests referencing verifyA2AHandshakeMessage,
verifyA2ASignedMessage, mockValidSignature, handshakeMessage, signedMessage,
unsignedMessage) — e.g. change "extracts issuer, nonce, and credential from a
valid handshake" to "returns issuer, nonce, and credential from a valid
handshake", "verifies the JWT with audience=self and issuer=counterparty" to
"requires JWT verification with audience=self and issuer=counterparty" or
similar, and similarly convert "rejects ..." to "throws ..." where appropriate;
apply consistent pattern across all test titles.
---
Nitpick comments:
In `@packages/ack-id/src/a2a/random.test.ts`:
- Around line 5-6: The current uuidPattern only checks generic UUID shape;
tighten it to assert UUIDv4 version and RFC-variant bits so tests for
generateRandomJti and generateRandomNonce validate v4 UUIDs. Replace the pattern
used in uuidPattern with one that enforces a '4' in the version position and one
of [89ab] in the variant position (e.g. pattern matching
...-4...-[89ab]...-...), keeping case handling consistent with the rest of the
tests.
In `@packages/ack-id/src/a2a/verify.test.ts`:
- Around line 84-112: The tests in verify.test.ts currently only assert that
malformed messages cause verifyA2AHandshakeMessage to reject, but they don’t
assert that JWT verification (verifyJwt) is not invoked; update the table-driven
test to mock or spy on the verifyJwt function used by verifyA2AHandshakeMessage
and add a non-call assertion (e.g., expect(verifyJwt).not.toHaveBeenCalled())
after the await expect(...).rejects.toThrow() so the test proves schema
validation occurs before any JWT verification. Make sure the mock/spy targets
the same symbol imported/used by verifyA2AHandshakeMessage (verifyJwt) so the
assertion reliably detects no calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4865f10f-67ef-40dd-9209-a61e6396a477
📒 Files selected for processing (5)
packages/ack-id/src/a2a/random.test.tspackages/ack-id/src/a2a/service-endpoints.test.tspackages/ack-id/src/a2a/sign-message.test.tspackages/ack-id/src/a2a/test-fixtures.tspackages/ack-id/src/a2a/verify.test.ts
Summary
packages/ack-id/src/a2a/module which previously had zero test coveragerandom.ts,service-endpoints.ts,sign-message.ts,verify.tsWhat's tested
Handshake signing — payload construction with nonce correlation for challenge-response flows, JWT wrapping into A2A data-part messages, metadata merge behavior (signature doesn't mess up existing metadata)
Handshake verification — JWT audience/issuer delegation to verifyJwt, schema rejection of malformed messages before any crypto/network calls, payload validation (missing fields, invalid DID URIs)
Signed message verification — content integrity check (signed payload must match message body), rejection of unsigned and improperly signed messages, contextId stripping (A2A servers inject this post-signing, verification must ignore it)
Utilities — UUID format and uniqueness for JTI/nonce generation, service endpoint construction with DID fragment assembly
Test plan
pnpm --filter ./packages/ack-id test— 39 tests passpnpm run check— format, types, lint all cleanSummary by CodeRabbit
AI Disclosure: This PR was developed with assistance from Claude Code (Claude Opus).